bump up mods.irisnet.org/modules/record to cosmos-sdk v0.50.10 #454
Conversation
WalkthroughThe changes in this pull request involve updates to import paths, dependency versions, and method signatures across multiple files in the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
modules/record/go.mod (2)
209-209: Address the TODO comment: Replace versionThere's a
TODOcomment indicating that the version needs to be replaced. Please update the version accordingly to ensure correct module references.Would you like assistance in updating the version here or opening a GitHub issue to track this task?
210-211: Specify explicit versions for local module replacementsCurrently, the
replacedirectives point to local paths without specifying versions. This may lead to unpredictability when the module is used elsewhere. It's recommended to specify explicit versions for better reproducibility.Consider updating the
replacedirectives as follows:replace ( - mods.irisnet.org/api => ../../api - mods.irisnet.org/simapp => ../../simapp + mods.irisnet.org/api => mods.irisnet.org/api vX.Y.Z + mods.irisnet.org/simapp => mods.irisnet.org/simapp vA.B.C )Replace
vX.Y.ZandvA.B.Cwith the appropriate version numbers.modules/record/types/expected_keepers.go (1)
1-19: Consider providing migration guidelines for interface changesThe transition from
sdk.Contexttocontext.Contextis a positive architectural change that aligns with Go best practices. However, this is a breaking change that affects multiple interfaces and their implementations.Consider:
- Documenting the migration process for downstream modules
- Providing examples of how to properly wrap/unwrap context when interfacing with older code
- Adding migration tests to verify compatibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
modules/record/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
modules/record/depinject.go(1 hunks)modules/record/go.mod(2 hunks)modules/record/keeper/depinject_test.go(2 hunks)modules/record/keeper/keeper.go(2 hunks)modules/record/keeper/keeper_test.go(1 hunks)modules/record/module.go(2 hunks)modules/record/simulation/operations.go(2 hunks)modules/record/types/expected_keepers.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- modules/record/depinject.go
🔇 Additional comments (9)
modules/record/go.mod (1)
3-3: Verify compatibility of dependencies with Go 1.21
Upgrading the Go version to 1.21 is a significant change. Ensure that all dependencies are compatible with Go 1.21 to prevent potential build or runtime issues.
Run the following script to check if any dependencies specify a lower Go version:
modules/record/types/expected_keepers.go (1)
4-5: LGTM: Standard context import added
The addition of the standard "context" import aligns with Go best practices and the cosmos-sdk v0.50.10 migration.
modules/record/keeper/keeper_test.go (2)
35-35: Verify import paths for codec package
The codec initialization change from NewAminoCodec to AppCodec is correct for cosmos-sdk v0.50.x. However, we should verify if the codec import path needs updating.
36-36: Verify context initialization API change
The simplified context initialization looks correct, but let's verify this change against the cosmos-sdk v0.50.x documentation.
✅ Verification successful
Context initialization needs to be updated for consistency
The codebase shows inconsistent patterns for context initialization in test files. While some files use the simplified NewContext(false), most test files still use the older pattern with tmproto.Header{}. This should be standardized across the codebase.
modules/record/keeper/keeper_test.go: Uses simplifiedNewContext(false)- Multiple other test files still use
NewContext(false, tmproto.Header{}):modules/token/keeper/keeper_test.gomodules/random/keeper/keeper_test.gomodules/oracle/keeper/keeper_test.go- And several others
Let's verify the correct API pattern in Cosmos SDK v0.50.x:
The simplified context initialization is correct
The codebase is using Cosmos SDK v0.50.10, and the simplified context initialization NewContext(false) in modules/record/keeper/keeper_test.go aligns with the SDK's v0.50.x changes. While other test files still use the older pattern with tmproto.Header{}, this is a transitional state and the simplified approach is the correct one moving forward.
- The change in
modules/record/keeper/keeper_test.gois correct and should serve as an example for updating other test files - Other modules (token, random, oracle, etc.) should be updated to use the same simplified pattern in future updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other context initialization patterns in test files
# to ensure consistency across the codebase
# Search for NewContext usage in test files
echo "Checking NewContext usage in tests:"
rg --type go "BaseApp.*NewContext.*test"
# Look for any remaining tmproto.Header usage
echo -e "\nChecking for tmproto.Header usage:"
rg --type go "tmproto\.Header"
Length of output: 2761
Script:
#!/bin/bash
# Check go.mod for cosmos-sdk version and search for NewContext documentation
echo "Checking cosmos-sdk version:"
grep "github.com/cosmos/cosmos-sdk" go.mod
# Look for NewContext definition in vendor or cached dependencies
echo -e "\nSearching for NewContext definition:"
rg --type go "func.*NewContext.*BaseApp"
Length of output: 281
Script:
#!/bin/bash
# Try different approach to find dependency information
echo "Checking for go.mod in repository:"
fd --type f "go.mod"
echo -e "\nChecking for BaseApp imports:"
rg --type go "\"github.com/cosmos/cosmos-sdk/baseapp\""
echo -e "\nLooking for NewContext usage patterns in test setup:"
rg --type go "func.*Setup.*suite" -A 10
Length of output: 2083
Script:
#!/bin/bash
# Check cosmos-sdk version in one of the go.mod files
echo "Checking cosmos-sdk version in simapp/go.mod:"
cat simapp/go.mod | grep "cosmos-sdk"
# Look for BaseApp.NewContext definition in test helpers
echo -e "\nChecking test helper implementations:"
rg --type go "func.*NewContext" simapp/test_helpers.go -A 5
Length of output: 471
modules/record/keeper/keeper.go (1)
66-68: LGTM! Iterator type updated correctly.
The method signature has been properly updated to use storetypes.Iterator instead of sdk.Iterator, maintaining the same functionality while using the new type system from cosmos-sdk v0.50.10.
modules/record/simulation/operations.go (2)
Line range hint 34-38: LGTM: Simplified parameter handling
The removal of the unused nil parameter in GetOrGenerate is a clean improvement that maintains the same functionality while simplifying the code.
Line range hint 3-19: Verify import paths for cosmos-sdk v0.50.10 compatibility
Some imports still use the github.com/cosmos/cosmos-sdk path. Let's verify if these need to be updated to the new cosmossdk.io paths as part of the v0.50.10 upgrade.
modules/record/module.go (1)
155-155: Verify validator updates handling in EndBlock.
The removal of []abci.ValidatorUpdate return type is a significant change. While the implementation is correct for this module, we should verify that validator updates are handled appropriately elsewhere if needed.
✅ Verification successful
The search results show that several other modules in the codebase (token, oracle, random, farm, htlc) are still using the old EndBlock signature with sdk.Context and returning []abci.ValidatorUpdate. This indicates an inconsistency in the module interfaces across the codebase. Let me verify if this is part of a migration pattern.
Let me try a different search strategy to understand the module interface changes.
Let me try one final search to check for module interface changes specifically in the cosmos-sdk imports.
EndBlock change is consistent with Cosmos SDK v0.50.10 upgrade
The changes in the EndBlock method signature are correct and aligned with Cosmos SDK v0.50.10:
- The module is using cosmos-sdk v0.50.10 as confirmed in go.mod
- The new EndBlock signature matches the updated module interface pattern
- Validator updates are no longer returned through EndBlock in v0.50.x
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other modules still use validator updates in EndBlock
rg -A 5 "func.*EndBlock.*ValidatorUpdate"
# Check for any new validator update handling patterns
ast-grep --pattern 'func $_ UpdateValidators($_) $_'
Length of output: 1897
Script:
#!/bin/bash
# Check for any recent changes or migrations in EndBlock signatures
git log -p --grep="EndBlock" -n 5
# Look for any migration guides or documentation
fd README.md | xargs rg -i "migration|upgrade|v0.50"
# Check if there are any new module interface definitions
rg -A 5 "type AppModule interface"
Length of output: 125
Script:
#!/bin/bash
# Check for any recent changes in module interfaces
rg -A 10 "interface" modules/record/
# Look for cosmos-sdk module interface definitions that might be imported
rg -l "cosmos.*/types.*AppModule"
# Check imports in the record module
rg "import \(" -A 10 modules/record/module.go
# Look for any migration documentation in docs
fd -e md | xargs rg -i "cosmos-sdk.*0.50"
Length of output: 15111
Script:
#!/bin/bash
# Check for any v0.50 related changes in go.mod
rg "cosmos-sdk.*v0.50" modules/record/go.mod
# Look for module interface changes in the codebase
rg -B 5 -A 5 "AppModule.*EndBlock" modules/record/
# Check for any migration files
fd migration modules/record/
Length of output: 820
modules/record/keeper/depinject_test.go (1)
Line range hint 60-64: LGTM: Module initialization order maintained correctly
The removal of the capability module from the genesis module order is correct, and the remaining module initialization sequence has been properly maintained. The critical ordering requirements are preserved:
- Genutils module remains after auth and staking
- Module dependencies are correctly ordered (e.g., staking before distribution)
| GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins | ||
| GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin | ||
| LockedCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins | ||
| SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins |
There was a problem hiding this comment.
💡 Codebase verification
Update BankKeeper implementations in multiple modules
The verification reveals several modules still using sdk.Context instead of context.Context in their BankKeeper interfaces and implementations:
modules/token/types/expected_keepers.go: Usessdk.ContextinGetBalanceandSpendableCoinsmodules/coinswap/types/expected_keepers.go: Usessdk.ContextinGetBalanceandSpendableCoinsmodules/mt/types/expected_keepers.go: Usessdk.Contextin all methodsmodules/htlc/types/expected_keepers.go: Usessdk.ContextinGetBalanceandSpendableCoinsmodules/oracle/types/expected_keepers.go: Usessdk.ContextinSpendableCoins
These modules need to be updated to maintain consistency with the new BankKeeper interface that uses context.Context.
🔗 Analysis chain
Verify BankKeeper implementations for context type changes
All BankKeeper methods have been updated to use context.Context instead of sdk.Context. This is a breaking change that requires verification of all implementations.
Let's verify all implementations have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of BankKeeper interface
# and verify they've been updated to use context.Context
# Search for BankKeeper implementations
echo "Searching for BankKeeper implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetAllBalances($_, $_) sdk.Coins {
$$$
}'
# Check for any remaining old-style implementations
echo "Checking for outdated implementations..."
rg -p "func.*\(.*sdk\.Context.*\).*GetAllBalances|GetBalance|LockedCoins|SpendableCoins"
Length of output: 9352
| ) | ||
|
|
||
| // AccountKeeper defines the expected account keeper for query account | ||
| type AccountKeeper interface { | ||
| GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI | ||
| GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Breaking changes in AccountKeeper interface require updates across multiple modules
The search results reveal that several modules still use the old signature with sdk.Context and authtypes.AccountI:
- modules/random/types/expected_keepers.go:14:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI - modules/oracle/types/expected_keepers.go:100:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI - modules/htlc/types/expected_keepers.go:39:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI - modules/farm/types/expected_keeper.go:39:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI - modules/mt/types/expected_keepers.go:10:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI - modules/coinswap/types/expected_keepers.go:42:
GetAccount(ctx sdk.Context, addr sdk.AccAddress) authtypes.AccountI
Only a few modules have been updated to use the new signature:
- modules/record/types/expected_keepers.go:11:
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI - modules/nft/types/expected_keepers.go:13:
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI - modules/service/types/expected_keeper.go:42:
GetAccount(ctx context.Context, addr sdk.AccAddress) sdk.AccountI
🔗 Analysis chain
Verify AccountKeeper implementations for breaking changes
The interface has two breaking changes:
- Context type changed from sdk.Context to context.Context
- Return type changed to sdk.AccountI
Let's verify all implementations of this interface:
Let me generate another script to specifically search for AccountKeeper implementations:
This script will help us find both updated and potentially outdated implementations of the GetAccount method, which is more specific to our review task than the previous search.
Let me try one more search with a different pattern to find AccountKeeper implementations:
This broader search should help us locate files related to AccountKeeper and examine the GetAccount method implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of AccountKeeper interface
# and verify they've been updated accordingly
# Search for AccountKeeper implementations
echo "Searching for AccountKeeper implementations..."
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) GetAccount($_, $_) $_ {
$$$
}'
# Search for any remaining old-style implementations
echo "Checking for outdated implementations..."
rg -p "GetAccount.*sdk\.Context.*authtypes\.AccountI"
Length of output: 9352
Script:
#!/bin/bash
# Search for AccountKeeper implementations and GetAccount method
echo "Searching for GetAccount implementations..."
rg -p "func.*GetAccount.*context\.Context.*sdk\.AccAddress.*sdk\.AccountI"
echo "Searching for potential old implementations..."
rg -p "func.*GetAccount.*sdk\.Context.*sdk\.AccAddress"
Length of output: 334
Script:
#!/bin/bash
# Search for files that might contain AccountKeeper implementations
echo "Searching for files containing AccountKeeper..."
fd -t f -e go | xargs rg "AccountKeeper" -l
echo "Examining GetAccount implementations..."
rg -p "GetAccount" -g "*.go" -A 3
Length of output: 20811
| "cosmossdk.io/log" | ||
| storetypes "cosmossdk.io/store/types" |
There was a problem hiding this comment.
💡 Codebase verification
Import paths need to be updated in multiple files
Several files still use the old import paths that need to be updated to the new modularized packages:
-
Store types import
github.com/cosmos/cosmos-sdk/store/typesneeds updating tocosmossdk.io/store/typesin:- modules/token/keeper/keeper.go
- modules/coinswap/keeper/keeper.go
- modules/random/keeper/keeper.go
- modules/oracle/keeper/keeper.go
- modules/farm/keeper/keeper.go
- modules/htlc/keeper/keeper.go
- modules/nft/migrations/v2/store_test.go
- Various depinject.go files
-
Logger import
github.com/cometbft/cometbft/libs/logneeds updating tocosmossdk.io/login:- modules/token/keeper/keeper.go
- modules/random/keeper/keeper.go
- modules/farm/keeper/keeper.go
- modules/htlc/keeper/keeper.go
- modules/coinswap/keeper/keeper.go
🔗 Analysis chain
LGTM! Import paths updated correctly.
The import paths have been updated to use the new modularized packages from cosmossdk.io, which is consistent with cosmos-sdk v0.50.10.
Let's verify if all necessary imports have been updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining old-style cosmos-sdk store imports
rg -l "github.com/cosmos/cosmos-sdk/store/types" --type go
# Check for any remaining old-style cosmos-sdk log imports
rg -l "github.com/cometbft/cometbft/libs/log" --type go
Length of output: 697
| @@ -108,7 +108,7 @@ func SimulateCreateRecord(ak types.AccountKeeper, bk types.BankKeeper) simtypes. | |||
| ), nil, err | |||
| } | |||
|
|
|||
| return simtypes.NewOperationMsg(msg, true, "simulate issue token", nil), nil, nil | |||
| return simtypes.NewOperationMsg(msg, true, "simulate issue token"), nil, nil | |||
There was a problem hiding this comment.
Fix incorrect operation message text
The operation message text "simulate issue token" is inconsistent with the actual operation being performed (record creation).
Apply this fix:
- return simtypes.NewOperationMsg(msg, true, "simulate issue token"), nil, nil
+ return simtypes.NewOperationMsg(msg, true, "simulate create record"), nil, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return simtypes.NewOperationMsg(msg, true, "simulate issue token"), nil, nil | |
| return simtypes.NewOperationMsg(msg, true, "simulate create record"), nil, nil |
| @@ -166,7 +164,7 @@ func (AppModule) GenerateGenesisState(simState *module.SimulationState) { | |||
| } | |||
|
|
|||
| // RegisterStoreDecoder registers a decoder for record module's types | |||
| func (am AppModule) RegisterStoreDecoder(sdr sdk.StoreDecoderRegistry) { | |||
| func (am AppModule) RegisterStoreDecoder(sdr simtypes.StoreDecoderRegistry) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of StoreDecoderRegistry type across modules
Several modules are still using the old sdk.StoreDecoderRegistry instead of simtypes.StoreDecoderRegistry:
modules/token/module.gomodules/random/module.gomodules/oracle/module.gomodules/htlc/module.gomodules/farm/module.gomodules/coinswap/module.go
These modules should be updated to use simtypes.StoreDecoderRegistry for consistency with cosmos-sdk v0.50.10's organization of simulation types.
🔗 Analysis chain
LGTM! Verify consistent usage of simtypes.StoreDecoderRegistry.
The change to use simtypes.StoreDecoderRegistry aligns with cosmos-sdk v0.50.10's organization of simulation types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent usage of StoreDecoderRegistry type
rg "StoreDecoderRegistry" --type go
# Check for any remaining old-style registry usage
rg "sdk\.StoreDecoderRegistry" --type go
Length of output: 1656
| @@ -149,12 +149,10 @@ | |||
| func (AppModule) ConsensusVersion() uint64 { return 1 } | |||
|
|
|||
| // BeginBlock performs a no-op. | |||
| func (AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {} | |||
| func (AppModule) BeginBlock(_ context.Context) error { return nil } | |||
There was a problem hiding this comment.
💡 Codebase verification
Migration to context.Context in BeginBlock is incomplete across modules
The verification reveals that several modules still use sdk.Context in their BeginBlock implementations:
modules/service/abci.gomodules/token/module.gomodules/random/module.gomodules/oracle/module.gomodules/htlc/module.gomodules/farm/module.go
These modules need to be updated to use context.Context and return error to maintain consistency with the cosmos-sdk v0.50.10 interface requirements.
🔗 Analysis chain
LGTM! Verify similar changes in related modules.
The change from sdk.Context to context.Context with error return aligns with cosmos-sdk v0.50.10's interface requirements.
The previous pattern might have been too specific. Let's try a broader search to understand the BeginBlock implementations across modules and verify if the migration from sdk.Context to context.Context is consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other modules have been similarly updated
ast-grep --pattern 'func (AppModule) BeginBlock(ctx context.Context) error'
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader pattern to catch BeginBlock implementations
ast-grep --pattern 'func $_($_ AppModule) BeginBlock($_ context.Context)'
# Also search for any remaining sdk.Context usage in BeginBlock
rg "BeginBlock.*sdk\.Context" -A 2
Length of output: 1889
| evidencetypes "cosmossdk.io/x/evidence/types" | ||
| "cosmossdk.io/x/feegrant" | ||
| upgradetypes "cosmossdk.io/x/upgrade/types" |
There was a problem hiding this comment.
💡 Codebase verification
Update remaining import paths to use cosmossdk.io namespace
Several files still use the old github.com/cosmos/cosmos-sdk import paths that need to be updated to cosmossdk.io:
modules/token/keeper/depinject_test.goe2e/sim_test.gomodules/oracle/keeper/depinject_test.gomodules/random/keeper/depinject_test.gomodules/coinswap/keeper/depinject_test.gomodules/farm/keeper/depinject_test.gomodules/htlc/keeper/depinject_test.goe2e/app_config.go
These imports should be updated to maintain consistency with the cosmos-sdk v0.50.10 upgrade and match the new import pattern already used in some files like simapp/app_v2.go and modules/record/keeper/depinject_test.go.
🔗 Analysis chain
Verify import path migrations across the codebase
The import paths have been correctly updated to use cosmossdk.io instead of github.com/cosmos/cosmos-sdk, which aligns with the cosmos-sdk v0.50.10 upgrade. However, we should verify that all related imports have been consistently updated across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths that need updating
# Search for any remaining github.com/cosmos/cosmos-sdk/x/{evidence,feegrant,upgrade} imports
echo "Checking for old import paths that need updating..."
rg "github\.com/cosmos/cosmos-sdk/x/(evidence|feegrant|upgrade)" --type go
# Verify all imports are using the new paths
echo "Verifying new import paths are used consistently..."
rg "cosmossdk\.io/x/(evidence|feegrant|upgrade)" --type go
Length of output: 3525
Closed: #444
Summary by CodeRabbit
Release Notes
New Features
Improvements
WeightedOperationsandSimulateCreateRecord.BeginBlockandEndBlockmethods for better control flow.Dependency Updates